Skip to content
This repository has been archived by the owner on Nov 1, 2023. It is now read-only.

Remove client_secret from config, add env var #1918

Merged
merged 4 commits into from
May 9, 2022

Conversation

ranweiler
Copy link
Member

@ranweiler ranweiler commented May 9, 2022

The client secret is only intended for confidential client use (e.g. ephemerally, in CI). If set via onefuzz config, it will be serialized as part of the config, for convenient reuse in that setting. When serialized, default filesystem permissions secure it against other local users, but this lacks defense in depth. On a long-lived host, this is brittle.

Make misuse impossible: remove the client_secret field from the config altogether. When initializing a Onefuzz API instance, check an env var for a client secret (to allow "setting" the client secret once across multiple commands in CI). This means every CLI invocation will also perform this env lookup. Continue to allow passing / overriding the secret as a CLI argument, too.

Tested with a dev service principal and client secret:

  • No secret (falls back on public client flow)
  • Secret only in env var
  • Secret only as parameter
  • Secret as parameter overrides invalid env var
  • Secret misplaced in config always ignored

@ranweiler ranweiler requested review from chkeita, stishkin and mgreisen May 9, 2022 21:33
src/cli/onefuzz/api.py Outdated Show resolved Hide resolved
@chkeita
Copy link
Contributor

chkeita commented May 9, 2022

This code that shows the command line to setup the onefuzz cli should be updated to remove the client_secret section.
Note this is most likely a noop because we never set the cli client_secret in the deployment

@ranweiler ranweiler merged commit 25405b0 into microsoft:main May 9, 2022
@ranweiler ranweiler deleted the env-client-secret branch May 9, 2022 23:58
@ghost ghost locked as resolved and limited conversation to collaborators Jun 9, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants